Phase 3 e2e: macOS leg and embedded-server isolation#979
Closed
notluquis wants to merge 18 commits into
Closed
Conversation
The e2e helper isolated only Electron userData via --user-data-dir. The
spawned Jupyter server still wrote runtime and config into the runner's
real ~/.jupyter and ~/Library/Jupyter, so a test run mutated the host and
leaked state between runs. src/main/server.ts builds the server env from
{ ...process.env }, so vars set in electron.launch({ env }) reach the
python process.
Redirect HOME plus the user-layer Jupyter dirs (JUPYTER_CONFIG_DIR,
JUPYTER_DATA_DIR, JUPYTER_RUNTIME_DIR) to a per-launch temp, mirroring
pytest-jupyter's jp_environ fixture. JUPYTER_PATH and JUPYTER_CONFIG_PATH
are deliberately left untouched: those drive the extension search path,
and the venv ships its jupyterlab assets under the env prefix, so
overriding them would hide the lab front end.
Add an e2e test that boots the labview and asserts the temp runtime dir
received a jpserver-*.json while the real runtime dir gained nothing, and
a unit test on the narrow server.ts seam that the spawn env sets
JUPYTER_CONFIG_DIR to the userData dir (or the JLAB_DESKTOP_CONFIG_DIR
override).
Note: AI-assisted (Claude Code). Manually verified: yarn test:unit 432/432 green; JLAB_TEST_PYTHON_PATH set, yarn test:e2e 6/6 green incl the new isolation test; mutation check on server.ts (hardcoding a wrong config dir fails both unit assertions); direct probe confirmed the temp runtime held jpserver-8387.json, jpserver-8387-open.html, jupyter_cookie_secret while real ~/Library/Jupyter/runtime gained 0 new entries (stayed at 237). yarn build fails on a pre-existing env issue unrelated to this change (stray @types/esrecurse 4.3.1 uses TS4.5 export syntax that the pinned tsc 4.2.4 cannot parse; reproduces on clean origin/master).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
macOS is the primary shipped platform but e2e only ran on Linux, so a whole class of platform/path bugs went uncaught in CI. The afterPack Linux-vs-macOS executable-name divergence is the canonical example: mac uses productFilename, Linux uses executableName, and only running the suite on both surfaces that kind of drift. Add a fail-fast:false os matrix (ubuntu-latest, macos-latest). The Linux-only steps stay gated so the ubuntu leg is byte-for-byte the same work as before: the Electron-binary unzip workaround (linux-x64 zip, dist/electron) and npx playwright install-deps are guarded with if: matrix.os == 'ubuntu-latest', and xvfb-run wraps the suite only when RUNNER_OS is Linux. macOS extracts dist/Electron.app via the normal postinstall and has a real display, so it needs neither. The venv provisioning and JLAB_TEST_PYTHON_PATH work unchanged on both. Artifacts are named per-OS so the two legs do not clobber each other. Note: AI-assisted (Claude Code). Manually verified: yaml parse (python yaml.safe_load) + that the ubuntu leg is unchanged (same steps, same commands, gates only add macOS); macOS leg verified by reading, not run (cannot run macOS CI locally).
62 tasks
The macOS e2e leg failed with "Electron failed to install correctly": the electron postinstall (extract-zip) drops the binary on the macOS runner too, not just Linux, so guarding the install step to ubuntu left macOS without a binary. Run install.js on every OS and keep the unzip repair as a Linux-only fallback. Note: AI-assisted (Claude Code). Manually verified: the workflow yaml parses; this mirrors the binary-install approach that the Linux leg already relied on, extended to macOS where the same postinstall gap appears in CI.
The macOS Playwright leg failed with "Electron failed to install correctly": extract-zip leaves dist without the app binary on the runner, the same way it does on Linux, but only the Linux leg had a repair step. Mirror that fallback for macOS using ditto, which preserves the Electron Framework symlinks that plain unzip would mangle. The step is conditional and logs whether it fired so the CI log shows the real on-disk state. Note: AI-assisted (Claude Code). Manually verified: reproduced ditto -x -k extraction of the cached darwin-arm64 zip locally (binary lands executable at Electron.app/Contents/MacOS/Electron as Mach-O arm64, Framework Current->A symlink preserved); confirmed local electron 27.0.2 install state matches isInstalled() expectations.
The repair step reports the binary present yet electron.launch still throws
"failed to install correctly". Dump path.txt, the resolved module path and
require('electron') output on the runner to find the real cause before
fixing. Diagnostic scaffolding, removed once the cause is known.
Note: AI-assisted (Claude Code). Manually verified: step is macOS-only and
non-fatal; reads on-disk state only.
…e abort) Note: AI-assisted (Claude Code). Manually verified: diagnostic-only, reads state.
The macOS Playwright leg failed with "Electron failed to install
correctly". CI diagnostics showed the real cause: dist/Electron.app and
dist/version are present and the binary is executable, but extract-zip never
wrote node_modules/electron/path.txt, and electron/index.js resolves the
executable from path.txt, not the binary. Always (re)write path.txt in the
repair step (its macOS value is constant), keeping the ditto re-extract only
for the case where the binary itself is missing. Verify resolution in-step
with require('electron').
Note: AI-assisted (Claude Code). Manually verified: diagnosed from CI logs
(path.txt missing, binary present, require('electron') stack at index.js:17);
ditto extraction of the cached darwin-arm64 zip reproduced locally.
The path.txt fix surfaced the real defect: extract-zip leaves the launcher in place but an incomplete Electron Framework.framework, so dyld fails with "Library not loaded: @rpath/Electron Framework.framework/Electron Framework" at launch. The previous repair skipped the re-extract when the launcher was present, so it never replaced the broken framework. Re-extract the cached darwin zip with ditto unconditionally (it reproduces a complete .app with the 144MB framework binary and the Versions/Current symlinks intact, verified locally) and assert both the launcher and the framework binary exist. Note: AI-assisted (Claude Code). Manually verified: ditto extraction of the cached darwin-arm64 zip locally yields the framework binary at Versions/A and the top-level Electron Framework symlink; diagnosed the dyld failure from the CI launch log.
The cache-root glob found nothing: @electron/get stores the zip under a content-hash subdirectory (~/Library/Caches/electron/<hash>/electron-...zip), not at the cache root, so 'ls .../electron-*-darwin-*.zip' failed. Ask @electron/get for the path directly (it returns the cached zip, downloading only if absent), then ditto-extract as before. Note: AI-assisted (Claude Code). Manually verified: ran the same downloadArtifact one-liner locally, it returned the hashed cache path.
…hook The helper set JUPYTER_CONFIG_DIR for isolation, but server.ts pins the spawned server's JUPYTER_CONFIG_DIR to JLAB_DESKTOP_CONFIG_DIR || getUserDataDir(), so that key never reached the server: its config landed in the app's --user-data-dir temp instead of jupyterDir. Not a leak (user-data-dir is also per-launch), but the key was dead and the comment overstated what it did. Set JLAB_DESKTOP_CONFIG_DIR (the app's documented override) so config co-locates in jupyterDir with data and runtime, keep JUPYTER_CONFIG_DIR for any child that reads it directly, and correct the comment. Note: AI-assisted (Claude Code). Manually verified: grep confirms server.ts:294 is the sole reader of JLAB_DESKTOP_CONFIG_DIR; ran the jupyter-isolation e2e locally against a real jupyterlab 4.5.6 (passes, 10.9s). A mutation check (removing JUPYTER_RUNTIME_DIR) showed runtime isolation is backstopped by JUPYTER_DATA_DIR, so I dropped a HOME-fallback assertion I had drafted that would have been tautological.
python-env.test.ts already covers New notebook. Add the other welcome-page session entry points, each booting a real Jupyter server and asserting the labview reaches the server URL: - New session (blank lab) - Open folder and Open file, with dialog.showOpenDialog stubbed to a temp path (the welcome page shows a unified Open action on macOS and split File/Folder actions elsewhere, so the link is selected by platform) - Connect to a running server: starts a second jupyter-lab from the seeded env on a reserved free port, opens the connect dialog, and submits the URL through the same setRemoteServerOptions IPC the dialog's Enter handler uses Note: AI-assisted (Claude Code). Manually verified: ran the full e2e suite locally against jupyterlab 4.5.6 with --workers=1 (the CI worker setting), 10 passed; the new four pass on their own too. Under the default local parallel workers these server-booting tests can flake from resource contention (the existing jupyter-isolation test does the same), which is why CI pins workers=1 with retries; env switching is not covered here since it needs two distinct interpreters to be meaningful.
Audit-driven robustness pass on the e2e suite: - Add pageByUrl and pageByLocator helpers. pageByUrl polls every open page's URL, so it catches an in-place navigation of an existing view (how the remote session loads) as well as a new window, unlike waitForWindowByUrl which only fires on a new-window event. Replace all waitForWindowByUrl uses with it. - Start the remote test server with python -m jupyterlab instead of the jupyter-lab launcher path so it resolves the same on Windows, macOS and Linux, isolate it to its own temp HOME / JUPYTER_* dirs, and capture spawn errors into the readiness loop instead of throwing from an event handler. - Find the connect dialog by a stable element (#server-url) and click a new #connect-remote-link id rather than matching the English link title. - Tighten the post-wait assertions from /:port/ to /lab so a navigation to a non-lab URL is caught. - Add the Windows runtime-dir branch to the isolation test. Note: AI-assisted (Claude Code). Manually verified: built and ran the full e2e suite locally against jupyterlab 4.5.6 with --workers=1 (the CI setting), 10 passed; confirmed python -m jupyterlab and the env has jupyterlab 4.5.6 plus notebook 7.5.5; verified the embedded server and the test server both pass --no-browser and that a full run opens no external browser (process count unchanged before/after). Patterns cross-referenced against JupyterLab Galata.
Move the booted-server URL regex (was duplicated inline in three tests and a local const in a fourth) and the 'set JLAB_TEST_PYTHON_PATH' skip reason into helpers as LAB_URL and NEEDS_PYTHON, and use them everywhere so every server-backed test waits on and skips for the same thing. No behavior change. Note: AI-assisted (Claude Code). Manually verified: full e2e suite green locally with --workers=1 (10 passed), eslint and prettier clean.
CI hit 'Target page has been closed' on the dialog.evaluate: the connect ThemedWindow loads asynchronously, so the page handle pageByLocator returns can go stale as the window navigates. Wrap the find-and-submit in expect().toPass() so it re-finds the dialog and resubmits until it sticks; evaluate runs once on success, so the setRemoteServerOptions IPC is sent at most once. Note: AI-assisted (Claude Code). Manually verified: ran remote-session 5x locally with clean state between runs, all passed; the earlier CI run flaked once on this exact evaluate call and recovered on retry, which this removes.
The second CI flake mode was 'no window contained selector #server-url': the connect dialog never opened because the click on the always-enabled remote link landed before the welcome page's scripts and electronAPI were ready, so the onclick was a no-op. Wait for #new-notebook-link to lose its disabled class first (the app's existing readiness signal, already used by the other server-backed tests) before clicking, so the click always reaches a wired page. Note: AI-assisted (Claude Code). Manually verified: ran remote-session 4x locally with clean process state between runs, all passed; the readiness wait matches the convention python-env and session-actions already use reliably.
Testing the bundled JupyterLab (notebooks, cells, the lab UI) is upstream's job via Galata; the desktop app's e2e should cover the Electron shell and that it correctly wraps a server, not re-test JupyterLab. Drop the extra server-booting tests (session actions, remote connect, the dedicated isolation test) and keep: - smoke: launch, welcome render, view composition, clean shutdown (no server) - python-env: New notebook boots a server and the labview reaches /lab (the one integration test, from #976) Keep the supporting infra that protects that one integration test: the per-launch HOME / JUPYTER_* redirect so it never writes the runner's real ~/.jupyter, the JLAB_DESKTOP_CONFIG_DIR override so config lands in the temp too, pageByUrl/pageByTitle, and the macOS leg with its electron-binary repair. Remove the now-unused pageByLocator helper and the test-only welcome link id. Note: AI-assisted (Claude Code). Manually verified: built and ran the reduced suite locally with --workers=1, 5 passed; eslint clean; welcomeview.ts is back to the master version (no test-only id left in production).
Collaborator
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part of #951 Phase 3 e2e hardening. Keeps the e2e suite scoped to the desktop shell plus a single integration test, rather than re-testing JupyterLab itself (that is upstream's job via Galata).
What the suite covers after this PR:
This PR adds:
Note: AI-assisted (Claude Code). Manually verified: built and ran the suite locally against jupyterlab 4.5.6 with --workers=1 (the CI setting), 5 passed; eslint clean; the app opens no external browser (it renders JupyterLab in the WebContentsView).